KCP e2e test#333
Conversation
|
Warning Rate limit exceeded@ntnn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
WalkthroughBumps KCP version and Makefile test deps; expands contrib/kcp docs and go.mod; adds comprehensive KCP e2e tests (backend bootstrap, Dex/OIDC client creation, browser binding, APIBinding flows); introduces test framework helpers (apply, kubeconfig, clients, logs, dex/backends); updates go.mod at repo root. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as TestHarness
participant Dex as Dex (OIDC)
participant KCP as KCP Server
participant BE as Backend
participant Prov as Provider WS
participant Cons as Consumer WS
participant Br as Browser (simulated)
rect rgb(235,245,255)
note over T: Bootstrap & provision
T->>Dex: CreateDexClient(addr) → (id, secret)
T->>KCP: bootstrapKCP()
T->>BE: StartBackend(..., dex id/secret, cookie keys)
end
rect rgb(242,255,240)
note over T,Prov: Provider setup
T->>Prov: create workspace, apply APIExport/CRDs
end
rect rgb(255,250,235)
note over T,Cons: Binding via browser
T->>Cons: dry-run bind (YAML)
Br->>Dex: authenticate (simulated)
Br->>KCP: callback → trigger full bind
T->>Cons: create APIBinding (with claims)
end
rect rgb(245,240,255)
note over T: Verification
T->>Cons: wait for CRD/resource presence
T->>Prov: verify resource synced
T-->>T: assert success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
contrib/kcp/README.md (3)
28-31: Dex invocation path likely wrong; align with Makefile or use make target.Prefer using the tool installed by the Makefile (hack/tools/dex) or the provided make target to avoid path drift.
Suggested snippet:
-./dex/bin/dex serve ./hack/dex-config-dev.yaml +make run-dex +# or explicitly: +# hack/tools/dex serve hack/dex-config-dev.yaml
148-152: Second consumer example mixes IDs/files; fix file name and make secret/namespace discovery generic.Use consistent file names and avoid hard-coded secret/namespace:
-./bin/kubectl-bind http://127.0.0.1:8080/clusters/2vgrh380y0cq38du/exports --dry-run -o yaml > apiserviceexport2.yaml -kubectl get secret kubeconfig-wvvsb -n kube-bind -o jsonpath='{.data.kubeconfig}' | base64 -d > remote2.kubeconfig -./bin/kubectl-bind apiservice -v 6 --remote-kubeconfig remote2.kubeconfig -f apiserviceexport.yaml --skip-konnector --remote-namespace kube-bind-m5zx4 +./bin/kubectl-bind "http://127.0.0.1:8080/clusters/${PROVIDER_CLUSTER_ID}/exports" --dry-run -o yaml > apiserviceexport2.yaml +kubectl get secrets -n kube-bind -o jsonpath='{.items[0].data.kubeconfig}' | base64 -d > remote2.kubeconfig +ns2=$(yq '.contexts[0].context.namespace' remote2.kubeconfig) +./bin/kubectl-bind apiservice -v 6 --remote-kubeconfig remote2.kubeconfig -f apiserviceexport2.yaml --skip-konnector --remote-namespace "$ns2"
162-167: Add language to fenced code block (markdownlint MD040).-``` +```bash export KUBECONFIG=.kcp/consumer.kubeconfig kubectl apply -f contrib/kcp/deploy/examples/cowboy.yaml kubectl apply -f contrib/kcp/deploy/examples/sheriff.yaml</blockquote></details> <details> <summary>contrib/kcp/test/e2e/logwriter.go (1)</summary><blockquote> `17-20`: **Trim trailing newline to avoid double-spacing in -v output.** ```diff func (lw *logWriter) Write(p []byte) (n int, err error) { - lw.tlog(lw.prefix + string(p)) + // avoid extra blank lines when p ends with '\n' + if len(p) > 0 && p[len(p)-1] == '\n' { + p = p[:len(p)-1] + } + lw.tlog(lw.prefix + string(p)) return len(p), nil }contrib/kcp/test/e2e/util.go (1)
24-26: Use NoErrorf for formatted message.-err := clientcmd.WriteToFile(*cfg, p) -require.NoError(t, err, "error writing kubeconfig to %q", p) +err := clientcmd.WriteToFile(*cfg, p) +require.NoErrorf(t, err, "error writing kubeconfig to %q", p)contrib/kcp/test/e2e/kcp_test.go (2)
174-185: Use NoErrorf for formatted error with output.-output, err := cmd.CombinedOutput() -require.NoError(t, err, "failed to apply file %q: %s", file, output) +output, err := cmd.CombinedOutput() +require.NoErrorf(t, err, "failed to apply file %q: %s", file, output)
137-153: Gosec G204: document intentional exec or suppress.If you keep execing local binaries, annotate to silence the false positive:
- backendCmd := exec.CommandContext( + // #nosec G204 -- executing a known local test binary with constant args + backendCmd := exec.CommandContext(contrib/kcp/go.mod (5)
34-34: cli-runtime version mismatch: align require with the rest.You require v0.32.0 while most k8s libs are v0.33.3 and you replace to a kcp-dev/kubernetes snapshot anyway. For clarity, bump the require to 0.33.3 so it matches peers (replace will still apply).
- k8s.io/cli-runtime v0.32.0 + k8s.io/cli-runtime v0.33.3Also applies to: 169-169
88-91: Mixed KCP submodule versions detected (pkg/apis v0.11.0 vs sdk v0.28.0). Align both submodule versions to the same major release or confirm that only one copy is pulled into the final build.
140-147: Consolidate JSON Patch usage
Multiple JSON Patch modules are imported and used across the codebase (v5 in pkg/committer; v4 in cmd/backend and k8s packages; v2 in backend and controller-runtime). Refactor to use one implementation (e.g. github.com/evanphx/json-patch/v5) exclusively and add go.mod replace directives to collapse the others.
11-17: Add tracking issues, rationale comments, and explicit excludes for each forked replace
Add a// TODOcomment linking the associated tracking issue and a brief rationale beside eachreplacedirective, and useexcludeblocks to lock these modules at your forked versions.
163-197: Add KCP Kubernetes snapshot header comment
Add a descriptive comment above the staging replace block in contrib/kcp/go.mod:// KCP Kubernetes snapshot at commit 20250816165010-ffe1d7c8649b – remove once upstream tags are released replace ( … )Group all k8s.io ⇒ github.com/kcp-dev/kubernetes/staging replaces under this single “KCP Kubernetes snapshot” block for easier grepping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Makefile(2 hunks)contrib/kcp/README.md(6 hunks)contrib/kcp/dex-config-dev.yaml(0 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/logwriter.go(1 hunks)contrib/kcp/test/e2e/util.go(1 hunks)hack/verify-go-versions.sh(0 hunks)
💤 Files with no reviewable changes (2)
- contrib/kcp/dex-config-dev.yaml
- hack/verify-go-versions.sh
🧰 Additional context used
🪛 GitHub Check: lint
contrib/kcp/test/e2e/kcp_test.go
[failure] 137-137:
G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
🪛 Gitleaks (8.28.0)
contrib/kcp/test/e2e/kcp_test.go
[high] 149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test-e2e
🔇 Additional comments (6)
Makefile (2)
80-80: Confirm release artifact for KCP v0.28.3 exists for all CI OS/ARCH combos.The curl URL in target $(KCP) will fail hard if the tag isn’t published for macOS/arm64 or linux/arm64. Please verify in CI images.
292-292: Good: verbose e2e output for contrib suites.This will surface t.Log and subcommand output during flakies.
contrib/kcp/README.md (1)
182-183: Undefined variable context for ‘remote.kubeconfig’ in Debug section.Either reference the earlier file path or add a command to create it in this section before using yq.
contrib/kcp/go.mod (3)
21-26: Ensure KCP module coherence in contrib/kcp
It looks like the version scan ran against the repo’s rootgo.mod, not thecontrib/kcp/go.modwhere you’ve pinned KCP to v0.28.1-0.20250926104223-cec2e15f24c6. Please rerun dependency resolution undercontrib/kcp(or with-modfile=contrib/kcp/go.mod) to confirm:
github.com/kcp-dev/kcpandgithub.com/kcp-dev/kcp/sdkresolve to the pinned commit- No stray
kcpversions are pulled in transitivelyclient-go,apimachinery,logicalcluster, andpkg/apisall align with your chosen KCP version
5-9: Add a guardrail comment for dev-only replace directives.
In contrib/kcp/go.mod, insert above thereplace (…)block a note that these paths (=> ../../…) are only for local e2e testing (built under contrib/kcp or via a dedicated go.work) and will break reproducible builds elsewhere. Consider moving them into a separate dev-only modfile. Then verify your CI actually runscd contrib/kcp && go mod tidy && go test ./…(or loads the go.work) so these replaces are applied as intended.
38-38: No replace overrides for sigs.k8s.io/yaml v1.4.0: confirmed there are noreplacedirectives in contrib/kcp/go.mod affecting this module, so it will use the official k8s-maintained repository.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
contrib/kcp/README.md (4)
26-38: Nice: explicit “Start kcp” and “Start dex” commands added.
122-122: Avoid hard-coded cluster ID; derive from “Get LogicalCluster”.Use the ID from the previous step.
-./bin/kubectl-bind http://127.0.0.1:8080/clusters/awsb9l59tt6xxwz3/exports --dry-run -o yaml > apiserviceexport.yaml +PROVIDER_CLUSTER_ID=<value from 'Get LogicalCluster'> +./bin/kubectl-bind "http://127.0.0.1:8080/clusters/${PROVIDER_CLUSTER_ID}/exports" --dry-run -o yaml > apiserviceexport.yaml
180-183: Good fix: jsonpath quoting now robust.
141-142: Trailing slash fixed for konnector binary.contrib/kcp/test/e2e/kcp_test.go (1)
335-339: Parse provider cluster URL robustly.Use net/url and validate segments before indexing. (Same concern noted previously.)
- providerClusterSplit := strings.Split(providerCluster.Status.URL, "/") - require.GreaterOrEqual(t, len(providerClusterSplit), 2, "Unexpected URL format: %s", providerCluster.Status.URL) - require.Equal(t, "clusters", providerClusterSplit[len(providerClusterSplit)-2], "Unexpected URL format: %s", providerCluster.Status.URL) - providerClusterID := providerClusterSplit[len(providerClusterSplit)-1] + u, err := url.Parse(providerCluster.Status.URL) + require.NoErrorf(t, err, "invalid provider cluster URL: %s", providerCluster.Status.URL) + parts := strings.Split(strings.Trim(u.Path, "/"), "/") + require.GreaterOrEqual(t, len(parts), 2, "Unexpected URL format: %s", providerCluster.Status.URL) + require.Equal(t, "clusters", parts[len(parts)-2], "Unexpected URL format: %s", providerCluster.Status.URL) + providerClusterID := parts[len(parts)-1] require.NotEmpty(t, providerClusterID, "Unexpected URL format: %s", providerCluster.Status.URL)Remember to add:
+ "net/url"to imports.
🧹 Nitpick comments (3)
contrib/kcp/README.md (1)
165-169: Specify language for fenced code block.Add bash for lint and readability. (MD040)
-``` +```bash export KUBECONFIG=.kcp/consumer.kubeconfig kubectl apply -f contrib/kcp/deploy/examples/cowboy.yaml kubectl apply -f contrib/kcp/deploy/examples/sheriff.yaml</blockquote></details> <details> <summary>contrib/kcp/test/e2e/kcp_test.go (1)</summary><blockquote> `145-152`: **Minor: remove stale TODO (“port should be dynamic”).** addr is already dynamic; the TODO can be dropped to avoid confusion. </blockquote></details> <details> <summary>contrib/kcp/go.mod (1)</summary><blockquote> `31-39`: **Align k8s.io/cli-runtime version**: bump the require in contrib/kcp/go.mod from v0.32.0 to v0.33.3 to match other k8s modules, then run `go mod tidy`. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fcd39197b9b453995e147111389626ac9f172a27 and 2db2538dcbfb6da353ba66b709bbb3923885cc14. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `contrib/kcp/go.sum` is excluded by `!**/*.sum` * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (8)</summary> * `contrib/kcp/README.md` (6 hunks) * `contrib/kcp/go.mod` (4 hunks) * `contrib/kcp/test/e2e/kcp_test.go` (1 hunks) * `contrib/kcp/test/e2e/logwriter.go` (1 hunks) * `contrib/kcp/test/e2e/util.go` (1 hunks) * `go.mod` (0 hunks) * `test/e2e/bind/happy-case_test.go` (1 hunks) * `test/e2e/framework/kubeconfig.go` (2 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * go.mod </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * contrib/kcp/test/e2e/logwriter.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>contrib/kcp/README.md</summary> 165-165: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)</summary> * GitHub Check: verify * GitHub Check: go-test * GitHub Check: lint * GitHub Check: go-test-e2e </details> <details> <summary>🔇 Additional comments (7)</summary><blockquote> <details> <summary>test/e2e/framework/kubeconfig.go (1)</summary><blockquote> `66-72`: **LGTM: handy helper for ephemeral kubeconfig files.** </blockquote></details> <details> <summary>test/e2e/bind/happy-case_test.go (1)</summary><blockquote> `26-26`: **No remaining gopkg.in/headzoo/surf.v1 references—module dedupe confirmed.** </blockquote></details> <details> <summary>contrib/kcp/go.mod (2)</summary><blockquote> `41-76`: **Verify cel-go replace version and ensure consistency** You currently require cel-go v0.23.2 (indirect) but have a replace directive to v0.22.0, while your module graph shows a transitive dependency on v0.22.1—confirm this downgrade is intentional and update the replace version to match the desired cel-go release. --- `19-21`: **Confirmed only github.com/headzoo/surf usage**. No gopkg.in/headzoo references remain across any go.mod. </blockquote></details> <details> <summary>contrib/kcp/test/e2e/util.go (3)</summary><blockquote> `17-27`: **LGTM!** Package declaration and imports are well-organized following Go conventions with standard library imports first, followed by third-party dependencies. --- `29-35`: **LGTM!** The function correctly obtains and deep-copies the kubeconfig. The deep copy ensures test isolation by preventing mutations from affecting other tests. --- `37-43`: **LGTM!** The function correctly writes the kubeconfig to a test-specific temporary directory and returns the path. Using `t.TempDir()` ensures automatic cleanup. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
contrib/kcp/README.md (2)
122-122: Hard-coded cluster ID remains.The bind URL still uses a hard-coded logical cluster ID (
awsb9l59tt6xxwz3) instead of using the ID obtained from the "Get LogicalCluster" step. This makes the example non-reusable.Update to use the cluster ID from step 8:
+PROVIDER_CLUSTER_ID=<value from 'Get LogicalCluster' step 8> -./bin/kubectl-bind http://127.0.0.1:8080/clusters/awsb9l59tt6xxwz3/exports --dry-run -o yaml > apiserviceexport.yaml +./bin/kubectl-bind "http://127.0.0.1:8080/clusters/${PROVIDER_CLUSTER_ID}/exports" --dry-run -o yaml > apiserviceexport.yaml
189-190: Invalid bash expansion syntax.The syntax
"$namespace-default"concatenates the variable with a literal-default, not a default value. Use parameter expansion for default values.Apply this fix:
-kubectl create cm provider -n "$namespace-default" -kubectl label cm provider app=wildwest -n "$namespace-default" +kubectl create cm provider -n "${namespace:-default}" +kubectl label cm provider app=wildwest -n "${namespace:-default}"contrib/kcp/go.mod (1)
29-29: Duplicate surf module remains unresolved.As flagged in the previous review, both
github.com/headzoo/surfandgopkg.in/headzoo/surf.v1appear in the dependency graph. This should be deduplicated to avoid maintaining two copies of the same module.Apply this replace directive to standardize on gopkg.in:
replace ( github.com/kube-bind/kube-bind => ../../ github.com/kube-bind/kube-bind/cli => ../../cli github.com/kube-bind/kube-bind/sdk => ../../sdk + github.com/headzoo/surf => gopkg.in/headzoo/surf.v1 v1.0.1 )Also applies to: 83-83
contrib/kcp/test/e2e/kcp_test.go (1)
315-320: Parse URL robustly to prevent panic.The URL splitting can panic if the URL doesn't have at least 2 segments. Use proper URL parsing to safely extract the cluster ID.
Apply this diff to parse the URL safely:
- providerClusterSplit := strings.Split(providerCluster.Status.URL, "/") - require.GreaterOrEqual(t, len(providerClusterSplit), 2, "Unexpected URL format: %s", providerCluster.Status.URL) - require.Equal(t, "clusters", providerClusterSplit[len(providerClusterSplit)-2], "Unexpected URL format: %s", providerCluster.Status.URL) - providerClusterID := providerClusterSplit[len(providerClusterSplit)-1] - require.NotEmpty(t, providerClusterID, "Unexpected URL format: %s", providerCluster.Status.URL) + u, err := url.Parse(providerCluster.Status.URL) + require.NoErrorf(t, err, "invalid provider cluster URL: %s", providerCluster.Status.URL) + parts := strings.Split(strings.Trim(u.Path, "/"), "/") + require.GreaterOrEqual(t, len(parts), 2, "Unexpected URL format: %s", providerCluster.Status.URL) + require.Equal(t, "clusters", parts[len(parts)-2], "Unexpected URL format: %s", providerCluster.Status.URL) + providerClusterID := parts[len(parts)-1] + require.NotEmpty(t, providerClusterID, "Unexpected URL format: %s", providerCluster.Status.URL)Add the import:
import ( "context" "fmt" "net" "net/http" + "net/url" "os"
🧹 Nitpick comments (5)
contrib/kcp/README.md (1)
166-166: Add language identifier to code fence.The code fence at line 166 is missing a language identifier, which reduces readability and syntax highlighting.
-``` +```bash export KUBECONFIG=.kcp/consumer.kubeconfigcontrib/kcp/test/e2e/kcp_test.go (4)
55-74: Validate Dex startup before returning.The function starts Dex but doesn't verify it's actually running and ready to accept requests. This could lead to race conditions where subsequent code tries to connect before Dex is ready.
Consider adding a readiness check:
require.NoError(t, dexCmd.Start()) t.Cleanup(func() { if dexCmd.Process != nil { t.Logf("Stopping dex (PID: %d)", dexCmd.Process.Pid) assert.NoError(t, dexCmd.Process.Kill()) } }) + + t.Log("Wait for Dex to be ready") + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:5556/dex/.well-known/openid-configuration", nil) + require.NoError(t, err) + kcptestinghelpers.Eventually(t, func() (bool, string) { + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false, "" + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK, "" + }, wait.ForeverTestTimeout, time.Millisecond*100) + t.Log("Dex is ready") }
155-166: Validate file existence before applying.The function doesn't check if the file exists before attempting to apply it, which could lead to confusing error messages.
Add a file existence check:
func applyFile(t testing.TB, kubeconfig, file string) { t.Helper() t.Logf("Applying file %q", file) + _, err := os.Stat(file) + require.NoError(t, err, "file %q does not exist", file) + // TODO: built a dynamic client to apply the files instead of using // kubectl cmd := exec.CommandContext(t.Context(), "kubectl", "apply", "-f", file) cmd.Env = append(os.Environ(), "KUBECONFIG="+kubeconfig) output, err := cmd.CombinedOutput() - require.NoError(t, err, "failed to apply file %q: %s", file, output) + require.NoErrorf(t, err, "failed to apply file %q: %s", file, output) }
265-265: Replace sleep with Eventually loop.The TODO indicates this sleep should be replaced with a proper wait condition. Using arbitrary sleeps in tests makes them flaky and slow.
Replace with an appropriate Eventually check for whatever resource or condition you're waiting for. For example, if waiting for backend readiness:
- time.Sleep(5 * time.Second) // TODO(ntnn): Replace with an eventually + // Wait for backend to stabilize any startup operations + t.Log("Waiting for backend to stabilize") + kcptestinghelpers.Eventually(t, func() (bool, string) { + // Add appropriate readiness check here + return true, "" + }, wait.ForeverTestTimeout, time.Millisecond*100)
328-328: Replace sleep with Eventually loop.The TODO indicates this sleep should be replaced with a proper wait condition. Using arbitrary sleeps in tests makes them flaky and slow.
Replace with an appropriate Eventually check for whatever resource or condition you're waiting for.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Makefile(2 hunks)contrib/kcp/README.md(6 hunks)contrib/kcp/bootstrap/config.go(1 hunks)contrib/kcp/bootstrap/config/core/bootstrap.go(1 hunks)contrib/kcp/bootstrap/server.go(5 hunks)contrib/kcp/cmd/kcp-init/main.go(1 hunks)contrib/kcp/deploy/bootstrap.go(1 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/util.go(1 hunks)test/e2e/framework/backend.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:55:45.015Z
Learnt from: ntnn
PR: kube-bind/kube-bind#333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.015Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: verify
- GitHub Check: lint
- GitHub Check: go-test
- GitHub Check: go-test-e2e
🔇 Additional comments (15)
contrib/kcp/bootstrap/config/core/bootstrap.go (1)
28-28: LGTM! Import path updated to reflect module reorganization.The import path change from
kcp/bootstrap/...tocontrib/kcp/bootstrap/...is consistent with the broader restructuring of the KCP-related code into the contrib directory.contrib/kcp/deploy/bootstrap.go (1)
35-35: LGTM! Import path updated consistently.The import path change aligns with the module reorganization moving KCP-related code to the contrib directory.
test/e2e/framework/backend.go (2)
45-45: LGTM! Improved test assertion.Replacing the panic with
require.NotEmptyis more idiomatic for test code and provides better error messages when key generation fails.
86-86: LGTM! Exported helper for reuse.Exporting
CreateDexClientallows other test packages (such as the new KCP e2e tests) to reuse this Dex client setup logic, which improves test infrastructure modularity.Also applies to: 107-107
contrib/kcp/go.mod (3)
1-1: LGTM! Module path updated for reorganization.The module path change from
kube-bind/kube-bind/kcptokube-bind/kube-bind/contrib/kcpaligns with the broader restructuring of KCP-related code into the contrib directory.
5-17: LGTM! Clear replace structure with TODO for fork cleanup.The grouped replace blocks are well-organized with clear separation between internal replacements and temporary forks. The TODO comment appropriately documents the intent to remove fork dependencies once they're no longer needed.
19-25: LGTM! KCP pseudo-version intentional.The comment clearly explains that the pseudo-version is pinned to a commit on main for features not yet in the v0.28.1 release, with plans to use versioned releases when v0.28.2 is available. This is appropriate for development and testing scenarios.
contrib/kcp/README.md (2)
26-38: LGTM! Clear preparation steps added.The new Preparation section provides explicit instructions for starting both Dex and KCP, addressing the previous review comment about missing startup commands.
156-156: File and namespace references are consistent
No changes needed.contrib/kcp/bootstrap/config.go (1)
28-28: LGTM! Import path updated consistently.The import path change for the options package aligns with the module reorganization to contrib/kcp.
contrib/kcp/cmd/kcp-init/main.go (1)
29-30: LGTM! Import paths updated for module reorganization.Both import paths are correctly updated to reference the contrib/kcp namespace, consistent with the broader restructuring.
contrib/kcp/bootstrap/server.go (2)
26-28: LGTM! Import paths updated consistently.The bootstrap config import paths are correctly updated to reference the contrib/kcp namespace.
43-77: Verify cumulative error collection is appropriate for bootstrap steps.The error handling changed from fail-fast (return on first error) to cumulative collection (continue executing all steps and return combined errors). If bootstrap steps have dependencies—for example, if bootstrapping the core workspace is required before bootstrapping the KCP workspace—executing later steps after earlier failures could produce confusing cascading errors.
Consider whether:
- Bootstrap steps are independent and can all be attempted even if one fails, or
- Bootstrap steps have dependencies that require early termination on failure
If steps are dependent, the previous fail-fast behavior was safer. If steps are independent and you want complete error reporting, the current approach is better.
Would you like me to review the bootstrap step implementations to check for inter-dependencies?
contrib/kcp/test/e2e/util.go (2)
46-94: LGTM! Backend setup follows best practices.The function properly generates ephemeral cookie keys per test run and correctly configures the backend server for KCP integration testing. The hard-coded Dex credentials are appropriate for test fixtures.
96-106: LGTM! Workspace config helper is correct.The function correctly constructs workspace-specific configurations for KCP testing. The URL construction pattern here (using
/clusters/for rest.Config.Host) is distinct from and complements the kubeconfig Server URL pattern (using:+ workspace base) used elsewhere in the tests.Based on learnings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contrib/kcp/README.md (1)
189-190: Invalid bash variable expansion remains unresolved.The syntax
"$namespace-default"concatenates the variable$namespacewith the literal string-default, rather than providing a default value. This will create namespace names likekube-bind-abc-defaultinstead of the intended fallback behavior.Apply this diff to use proper default-value expansion:
-kubectl create cm provider -n "$namespace-default" -kubectl label cm provider app=wildwest -n "$namespace-default" +kubectl create cm provider -n "${namespace:-default}" +kubectl label cm provider app=wildwest -n "${namespace:-default}"
🧹 Nitpick comments (1)
contrib/kcp/test/e2e/kcp_test.go (1)
108-113: Consider using net/url.Parse for more robust URL parsing.While the current string splitting approach now includes a length check to prevent panics, using
net/url.Parsewould be more robust and handle edge cases (encoded characters, query parameters, etc.) more gracefully.Apply this diff to use proper URL parsing:
+ u, err := url.Parse(providerCluster.Status.URL) + require.NoErrorf(t, err, "invalid provider cluster URL: %s", providerCluster.Status.URL) + parts := strings.Split(strings.Trim(u.Path, "/"), "/") - providerClusterSplit := strings.Split(providerCluster.Status.URL, "/") - require.GreaterOrEqual(t, len(providerClusterSplit), 2, "Unexpected URL format: %s", providerCluster.Status.URL) - require.Equal(t, "clusters", providerClusterSplit[len(providerClusterSplit)-2], "Unexpected URL format: %s", providerCluster.Status.URL) - providerClusterID := providerClusterSplit[len(providerClusterSplit)-1] + require.GreaterOrEqual(t, len(parts), 2, "Unexpected URL format: %s", providerCluster.Status.URL) + require.Equal(t, "clusters", parts[len(parts)-2], "Unexpected URL format: %s", providerCluster.Status.URL) + providerClusterID := parts[len(parts)-1] require.NotEmpty(t, providerClusterID, "Unexpected URL format: %s", providerCluster.Status.URL)Add import:
import ( "fmt" + "net/url" "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
Makefile(2 hunks)contrib/kcp/README.md(6 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/logwriter.go(1 hunks)contrib/kcp/test/e2e/util.go(1 hunks)test/e2e/framework/kubeconfig.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/framework/kubeconfig.go
- contrib/kcp/test/e2e/logwriter.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
PR: kube-bind/kube-bind#333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
Makefile (2)
80-80: LGTM! KCP version bump to v0.28.3.The patch version update from v0.28.0 to v0.28.3 brings bug fixes and stability improvements from the 0.28.x release line, which should improve test reliability.
Based on learnings
289-291: LGTM! Test dependencies and verbosity improvements.The addition of DEX dependency aligns with the backend's OIDC authentication requirements, and the verbose flag (
-v) will provide better visibility into the e2e test execution flow.contrib/kcp/README.md (1)
26-202: Excellent documentation improvements!The restructured README provides clear, step-by-step instructions for the KCP integration workflow. The logical flow from preparation through testing and debugging, along with the added context about running processes, significantly improves usability.
contrib/kcp/go.mod (2)
11-32: Temporary dependency pinning is well-documented.The replace blocks for KCP modules and forked dependencies are clearly explained with comments. Pinning to a specific commit for testing unreleased features is a reasonable approach.
Consider tracking the status of these temporary replacements:
- Monitor when v0.28.2 is released to switch from the commit-based version
- Track upstream PRs for the forked multicluster-provider and multicluster-runtime fixes
Do you want me to generate a script to check for the availability of v0.28.2 and newer versions?
34-171: Appropriate dependency additions for e2e testing.The new dependencies support the comprehensive e2e test implementation:
gorilla/securecookie: For secure cookie generation in backend testsheadzoo/surf: For browser automation during binding simulationtestify: For enhanced test assertions- Additional indirect dependencies required by the KCP testing framework
contrib/kcp/test/e2e/kcp_test.go (1)
40-214: Well-structured comprehensive e2e test!The test provides excellent coverage of the KCP integration workflow:
- Clear separation of setup phases (Dex, KCP, backend, workspaces)
- Good use of helper functions for modularity
- Comprehensive sync verification with table-driven subtests covering create, update, and delete operations
- Appropriate use of
Eventuallyfor async operationsThe overall design follows testing best practices.
contrib/kcp/test/e2e/util.go (3)
63-113: Excellent helper for APIBinding generation.The
generateApiBindinghelper properly validates permission claims and handles the Kubernetes core group convention (converting "core" to empty string). The validation prevents malformed claims from causing runtime errors.
296-306: LGTM! Proper workspace-targeted configuration.The helper correctly constructs workspace-scoped REST config and kubeconfig paths using the
/clusters/<workspace>URL pattern for workspace targeting.Based on learnings
140-395: Well-crafted helper functions for e2e testing.The helper functions provide excellent support for the KCP e2e tests:
bootstrapKCP: Properly initializes KCP using kcp-initbootstrapBackend: Robust backend startup with health checksapplyFile: Functional kubectl-based resource application (TODO noted for future enhancement)performBindingWithBrowser: Well-structured binding flow with dry-run validationsimulateKCPBrowser: Clean browser automation for OAuth flowstoUnstructured: Simple and effective YAML conversionAll helpers follow testing best practices with
t.Helper()calls, clear logging, and appropriate error handling.
ntnn
force-pushed
the
bind296
branch
4 times, most recently
from
2b145fa to
be4e966
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
contrib/kcp/README.md (3)
122-122: Parameterize the provider cluster URL before binding.Hard-coding specific cluster IDs makes these commands unusable outside your local run and reintroduces the issue called out previously. Please derive the URL from the current logicalcluster before invoking
kubectl-bind, and reuse it for the second consumer section as well.Apply this diff to resolve both occurrences:
-./bin/kubectl-bind http://127.0.0.1:8080/clusters/awsb9l59tt6xxwz3/exports --dry-run -o yaml > apiserviceexport.yaml +PROVIDER_CLUSTER_URL=$(kubectl get logicalcluster cluster -o jsonpath='{.status.url}') +./bin/kubectl-bind "${PROVIDER_CLUSTER_URL}/exports" --dry-run -o yaml > apiserviceexport.yaml @@ -./bin/kubectl-bind http://127.0.0.1:8080/clusters/2vgrh380y0cq38du/exports --dry-run -o yaml > apiserviceexport2.yaml +PROVIDER_CLUSTER_URL=$(kubectl get logicalcluster cluster -o jsonpath='{.status.url}') +./bin/kubectl-bind "${PROVIDER_CLUSTER_URL}/exports" --dry-run -o yaml > apiserviceexport2.yamlAlso applies to: 152-152
189-190: Fix${namespace-default}expansion.
"$namespace-default"produces the literal string with a dash, not a default value. This was reported earlier and still needs to be corrected to${namespace:-default}.Apply this diff:
-kubectl create cm provider -n "$namespace-default" -kubectl label cm provider app=wildwest -n "$namespace-default" +kubectl create cm provider -n "${namespace:-default}" +kubectl label cm provider app=wildwest -n "${namespace:-default}"
167-170: Add the shell language marker to this block.The fenced block still lacks a language identifier, triggering markdownlint MD040 and reducing readability. Please tag it as bash, as previously requested.
Apply this diff:
-``` +```bash export KUBECONFIG=.kcp/consumer.kubeconfig kubectl apply -f contrib/kcp/deploy/examples/cowboy.yaml kubectl apply -f contrib/kcp/deploy/examples/sheriff.yaml</blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>contrib/kcp/test/e2e/logwriter.go (1)</summary><blockquote> `21-24`: **LGTM! Consider adding godoc comments.** The type design is clean and appropriate for wrapping test logging functionality. Optionally, add a godoc comment to document the purpose: ```diff +// logWriter wraps testing.TB.Log to implement io.Writer with a message prefix. type logWriter struct { prefix string tlog func(args ...any) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
Makefile(2 hunks)contrib/kcp/README.md(6 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/backend.go(1 hunks)contrib/kcp/test/e2e/browser.go(1 hunks)contrib/kcp/test/e2e/kcp.go(1 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/logwriter.go(1 hunks)go.mod(1 hunks)test/e2e/framework/apply.go(1 hunks)test/e2e/framework/clients.go(1 hunks)test/e2e/framework/kubeconfig.go(2 hunks)test/e2e/framework/logs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/kcp/test/e2e/kcp_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
contrib/kcp/test/e2e/kcp.go (1)
test/e2e/framework/kubeconfig.go (2)
WriteKubeconfig(66-72)RestToKubeconfig(40-64)
contrib/kcp/test/e2e/backend.go (2)
test/e2e/framework/dex.go (1)
CreateDexClient(83-112)backend/options/options.go (2)
SchemaSource(108-108)CustomResourceDefinitionSource(116-116)
contrib/kcp/test/e2e/browser.go (3)
test/e2e/framework/bind.go (3)
Bind(36-75)SubCommandInvocation(77-81)BindAPIService(83-105)test/e2e/framework/clients.go (1)
ApiextensionsClient(43-47)test/e2e/framework/browser.go (1)
BrowserEventuallyAtPath(29-38)
test/e2e/framework/apply.go (1)
test/e2e/framework/clients.go (2)
DynamicClient(31-35)DiscoveryClient(49-53)
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: verify
- GitHub Check: go-test
- GitHub Check: lint
- GitHub Check: go-test-e2e
🔇 Additional comments (3)
contrib/kcp/test/e2e/logwriter.go (3)
1-19: LGTM!The license header, package declaration, and imports are correct and follow standard conventions.
26-31: LGTM!The constructor correctly initializes the logWriter, and using
testing.TBmakes it compatible with both*testing.Tand*testing.B.
33-36: LGTM!The
Writemethod correctly implements theio.Writerinterface. The implementation is appropriate for test logging:
- Returns the correct length as required by the interface contract
t.Logis safe for concurrent use, so no mutex is needed- Bytes-to-string conversion is acceptable for logging purposes
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
ntnn
force-pushed
the
bind296
branch
2 times, most recently
from
f9c3013 to
d71186e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contrib/kcp/README.md (1)
184-200: Invalid bash variable expansion syntax.The syntax
"$namespace-default"is incorrect and will produce a literal string with a dash appended (e.g., ifnamespaceis empty, you get-default, notdefault). Use"${namespace:-default}"for default-value expansion.Apply this diff:
-kubectl create cm provider -n "$namespace-default" -kubectl label cm provider app=wildwest -n "$namespace-default" +kubectl create cm provider -n "${namespace:-default}" +kubectl label cm provider app=wildwest -n "${namespace:-default}"
🧹 Nitpick comments (6)
contrib/kcp/test/e2e/logwriter.go (2)
26-31: LGTM! Consider optional validation.The constructor is straightforward and correct. For added robustness, you could add a nil check on the
testing.TBparameter, though panics from nil derefs are acceptable in test infrastructure.Optional: Add validation for robustness:
func newLogWriter(prefix string, t testing.TB) *logWriter { + if t == nil { + panic("newLogWriter: testing.TB cannot be nil") + } return &logWriter{ prefix: prefix, tlog: t.Log, } }
33-36: LGTM! Consider refactoring for efficiency and cleaner output.The implementation correctly forwards writes to the test logger. For better performance and output formatting, consider using
strings.Builderand trimming trailing newlines.Refactor to reduce allocations and handle newlines:
+import ( + "strings" + "testing" +) + func (lw *logWriter) Write(p []byte) (n int, err error) { - lw.tlog(lw.prefix + string(p)) + msg := strings.TrimSuffix(string(p), "\n") + lw.tlog(lw.prefix + msg) return len(p), nil }This avoids double newlines when the input already has a trailing newline (common with many loggers).
contrib/kcp/test/e2e/browser.go (1)
45-49: Force YAML output in dry-run and assert non-empty stdoutMake the test resilient to output-format defaults and ensure something was produced.
- iostreams, _, bufOut, _ := genericclioptions.NewTestIOStreams() - framework.Bind(t, iostreams, authURLDryRunCh, nil, bindURL, "--kubeconfig", consumerKubeconfig, "--dry-run") + iostreams, _, bufOut, _ := genericclioptions.NewTestIOStreams() + framework.Bind(t, iostreams, authURLDryRunCh, nil, bindURL, "--kubeconfig", consumerKubeconfig, "--dry-run", "-o", "yaml") _, err := yaml.YAMLToJSON(bufOut.Bytes()) require.NoError(t, err, "Generated output is not valid YAML") + require.NotEmpty(t, bufOut.Bytes(), "No output produced in dry-run")contrib/kcp/test/e2e/kcp.go (3)
19-39: Use url.JoinPath to build workspace-scoped HostAvoid manual string concatenation that can yield malformed URLs.
import ( "fmt" + "net/url" "strings" "testing" "time" @@ func wsConfig(t testing.TB, server kcptestingserver.RunningServer, workspace logicalcluster.Path) (*rest.Config, string) { cfg := server.BaseConfig(t) - cfg.Host += "/clusters/" + workspace.String() + joined, err := url.JoinPath(cfg.Host, "clusters", workspace.String()) + require.NoError(t, err) + cfg.Host = joinedAlso applies to: 41-47
70-77: Prefer strings.Cut over SplitN for claim parsingSimpler and avoids slice allocation.
- for _, claim := range acceptPermissionClaims { - split := strings.SplitN(claim, ".", 2) - require.Len(t, split, 2, "invalid permission claim: %s, must be in the form <resource>.<group>", claim) - - resource, group := split[0], split[1] + for _, claim := range acceptPermissionClaims { + resource, group, ok := strings.Cut(claim, ".") + require.True(t, ok, "invalid permission claim: %s, must be in the form <resource>.<group>", claim)
139-147: Avoid shadowing the imported options packageRename the local variable for clarity.
- fs := pflag.NewFlagSet("kcp-bootstrap", pflag.ContinueOnError) - options := options.NewOptions() - options.AddFlags(fs) + fs := pflag.NewFlagSet("kcp-bootstrap", pflag.ContinueOnError) + opts := options.NewOptions() + opts.AddFlags(fs) @@ - options.KCPKubeConfig = adminKubeconfig + opts.KCPKubeConfig = adminKubeconfig @@ - completed, err := options.Complete() + completed, err := opts.Complete()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contrib/kcp/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
Makefile(2 hunks)contrib/kcp/README.md(5 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/backend.go(1 hunks)contrib/kcp/test/e2e/browser.go(1 hunks)contrib/kcp/test/e2e/kcp.go(1 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/logwriter.go(1 hunks)go.mod(2 hunks)test/e2e/framework/apply.go(1 hunks)test/e2e/framework/backend.go(2 hunks)test/e2e/framework/clients.go(1 hunks)test/e2e/framework/dex.go(6 hunks)test/e2e/framework/dex_linux.go(1 hunks)test/e2e/framework/kubeconfig.go(2 hunks)test/e2e/framework/logs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Makefile
- test/e2e/framework/logs.go
- test/e2e/framework/clients.go
- go.mod
- test/e2e/framework/apply.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
PR: kube-bind/kube-bind#333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
🧬 Code graph analysis (4)
contrib/kcp/test/e2e/browser.go (3)
test/e2e/framework/bind.go (3)
Bind(36-75)SubCommandInvocation(77-81)BindAPIService(83-105)test/e2e/framework/clients.go (1)
ApiextensionsClient(43-47)test/e2e/framework/browser.go (1)
BrowserEventuallyAtPath(29-38)
contrib/kcp/test/e2e/backend.go (3)
test/e2e/framework/dex.go (1)
CreateDexClient(88-122)backend/options/options.go (2)
SchemaSource(108-108)CustomResourceDefinitionSource(116-116)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
InformerScope(47-47)
contrib/kcp/test/e2e/kcp_test.go (5)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
InformerScope(47-47)test/e2e/framework/dex.go (1)
StartDex(51-86)test/e2e/framework/konnector.go (1)
StartKonnector(35-75)test/e2e/framework/apply.go (1)
ApplyFiles(46-69)test/e2e/framework/clients.go (1)
DynamicClient(31-35)
contrib/kcp/test/e2e/kcp.go (1)
test/e2e/framework/kubeconfig.go (2)
WriteKubeconfig(66-72)RestToKubeconfig(40-64)
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: verify
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: go-test
🔇 Additional comments (18)
contrib/kcp/test/e2e/logwriter.go (2)
17-19: LGTM!Package declaration and imports are clean and appropriate for e2e test infrastructure.
21-24: LGTM!The struct design is clean and appropriate. Storing the log function as a field provides flexibility for testing.
contrib/kcp/go.mod (2)
11-14: LGTM: Helpful documentation for the temporary SDK pin.The comment clearly explains why the SDK replacement is needed and when it can be removed. This improves maintainability.
17-34: LGTM: Dependencies align with e2e test framework expansion.The new dependencies support the KCP e2e tests:
securecookieandsurfenable browser-based binding teststestifyprovides test assertionscli-runtimeandyamlsupport Kubernetes manifest handlingNote: A previous review comment already flagged the duplicate surf module paths (github.com vs gopkg.in); see past comments.
test/e2e/framework/dex_linux.go (1)
25-30: LGTM: Correct Linux-specific process termination.The use of
Pdeathsig: SIGKILLensures Dex processes are cleaned up when the parent test process exits on Linux. This complements the cross-platformdexKillstub intest/e2e/framework/dex.go.test/e2e/framework/kubeconfig.go (1)
66-72: LGTM: Clean helper for test kubeconfig management.The function properly:
- Marks itself as a helper
- Uses test temp directory for isolation
- Handles errors appropriately
- Returns the path for downstream use
test/e2e/framework/backend.go (2)
41-47: LGTM: Cookie encryption key properly generated.The addition of encryption key generation follows the same secure pattern as the signing key, using
securecookie.GenerateRandomKey(32)to create ephemeral per-test keys.
79-81: LGTM: Dynamic Dex credential provisioning.Replacing hard-coded OIDC credentials with dynamically generated ones via
CreateDexClientimproves security and eliminates static secrets. This aligns with the updated signature returning(string, string)for the client ID and secret.contrib/kcp/test/e2e/kcp_test.go (3)
41-49: LGTM: Well-structured test entry points.The two test functions provide coverage for both cluster-scoped and namespace-scoped resources, using
t.Parallel()for efficient execution.
51-153: LGTM: Comprehensive KCP integration test orchestration.The test flow is well-organized:
- Dex setup
- KCP server bootstrap
- Consumer workspace creation and konnector setup
- Backend bootstrap
- Provider workspace setup with API bindings
- Browser-based binding flow
- Resource synchronization tests
The URL parsing fix (lines 126-134) properly validates the split before indexing, addressing the previous review concern.
Based on learnings
165-243: LGTM: Thorough resource synchronization testing.The test coverage includes:
- Downstream creation syncing upstream
- Spec updates syncing upstream
- Downstream deletion syncing upstream
The helper function
testKcpClientproperly handles both cluster-scoped and namespace-scoped resources, with appropriate namespace handling.test/e2e/framework/dex.go (2)
42-49: LGTM: Cross-platform process termination approach.The stub implementation with a comment explaining the limitation is clear. Platform-specific implementations (like
dex_linux.go) provide actual cleanup where supported.
88-121: LGTM: Secure credential generation with updated signature.The changes improve the design:
- Returns
(id, secret)for use by callers- Generates a secure random secret via
rand.Text()- Creates a unique client ID based on the port
- Cleanup properly references the generated ID
This enables dynamic Dex client provisioning in backend tests.
contrib/kcp/test/e2e/backend.go (2)
51-137: LGTM: Flexible backend startup with proper port allocation.The implementation:
- Supports both prebuilt binary and in-process modes
- Properly allocates ephemeral ports (lines 69-72)
- Uses dynamic Dex credential provisioning (line 75)
- Generates secure cookie keys per test
- Registers cleanup handlers
Note: The past review concern about hard-coded ports has been addressed by using
net.Listen("tcp", "localhost:0").
139-190: LGTM: Well-orchestrated backend bootstrap flow.The function:
- Polls for the APIExportEndpointSlice to become available
- Generates kubeconfig for the backend
- Starts the backend with appropriate KCP-specific flags
- Polls the health endpoint until ready
The health check (lines 177-186) uses
http.DefaultClient.Doappropriately rather than relying on externalcurl.contrib/kcp/README.md (3)
28-38: LGTM: Clear preparation steps.The addition of explicit "Preparation" section with
makecommands improves usability.
75-90: LGTM: Updated workspace and binding commands.The documentation correctly shows:
kubectl create-workspacesyntax- Multi-line
--accept-permission-claimfor better readability
122-131: LGTM: Dynamic namespace extraction.Using
yqto extract the namespace from the kubeconfig is more robust than hard-coding, and properly passing it to--remote-namespaceensures correct binding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/framework/dex.go (2)
119-119: Avoid duplicating id generation logic.The cleanup function re-constructs the id using
"kube-bind-" + portinstead of reusing theidvariable from the outer scope. This creates maintenance burden if the id format changes.Apply this diff to reuse the
idvariable:- _, err = dexapi.NewDexClient(conn).DeleteClient(ctx, &dexapi.DeleteClientReq{Id: "kube-bind-" + port}) + _, err = dexapi.NewDexClient(conn).DeleteClient(ctx, &dexapi.DeleteClientReq{Id: id})
43-51: Consider documenting process leak implications.The no-op
dexKillon non-Linux platforms allows Dex processes to leak, which is acknowledged in comments but may cause resource exhaustion in CI or during repeated test runs.Consider adding a test cleanup mechanism or documenting how developers should manually clean up leaked Dex processes on macOS/Windows. Alternatively, explore using
t.Cleanup()withcmd.Process.Kill()as a cross-platform fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/e2e/framework/dex.go(6 hunks)test/e2e/framework/dex_linux.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: verify
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: go-test
🔇 Additional comments (2)
test/e2e/framework/dex_linux.go (1)
25-32: LGTM! Clean platform-specific implementation.The init hook correctly wires
Pdeathsigfor Linux to ensure child Dex processes are terminated when the parent test process exits. The use of the_linux.gofile suffix for platform-specific code follows Go conventions.test/e2e/framework/dex.go (1)
100-100: Verifyrand.Text()exists or update import
The standardcrypto/randpackage doesn’t exportText(). Ensure you’ve imported or defined a customrand.Text()helper, or replace this call with the correct function.
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>
ntnn
force-pushed
the
bind296
branch
3 times, most recently
from
15271c6 to
55f4784
Compare
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
ntnn
force-pushed
the
bind296
branch
2 times, most recently
from
db90dcd to
b082874
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contrib/kcp/README.md (1)
122-131: Secret selection must be deterministic.
kubectl get secrets ... -o jsonpath='{.items[0].data.kubeconfig}'grabs whichever secret happens to be first in the list. As soon as another secret exists inkube-bind(for example after adding a second consumer), this command will decode the wrong kubeconfig and the bind step fails. Address the secret by name (or filter via a label) instead of relying on.items[0].
♻️ Duplicate comments (1)
contrib/kcp/README.md (1)
188-189: Fix invalid bash expansion.
"$namespace-default"always produces a literal with a dash (e.g.foo-default) instead of defaulting to"default"whennamespaceis empty. Use${namespace:-default}so the namespace falls back correctly.Apply this diff:
-kubectl create cm provider -n "$namespace-default" -kubectl label cm provider app=wildwest -n "$namespace-default" +kubectl create cm provider -n "${namespace:-default}" +kubectl label cm provider app=wildwest -n "${namespace:-default}"
🧹 Nitpick comments (3)
test/e2e/framework/logs.go (1)
25-26: LGTM! Good practice for test framework initialization.The controller-runtime logger configuration appropriately prevents warnings during e2e test execution. Setting up the logger in the
init()function ensures it's configured before any tests run, and usingUseDevMode(true)is appropriate for the test environment.Optional consideration:
The existing component-base logging is configured with
Verbosity = logsv1.VerbosityLevel(2), while the Zap logger relies on dev mode defaults. If you need consistent verbosity across both logging systems, you could optionally configure the Zap logger with an explicit verbosity level usingzap.Level(), though the dev mode defaults should be suitable for e2e tests.Also applies to: 39-42
contrib/kcp/test/e2e/kcp_test.go (1)
155-163: Consider clarifying the client scoping logic.The helper correctly returns a namespaced or non-namespaced client based on scope and namespace parameters. The logic is sound, but the intent might be clearer with a brief comment explaining when each path is taken.
Consider adding a clarifying comment:
func testKcpClient(t testing.TB, cfg *rest.Config, scope kubebindv1alpha2.InformerScope, gvr schema.GroupVersionResource, namespace string) dynamic.ResourceInterface { t.Helper() client := framework.DynamicClient(t, cfg).Resource(gvr) + // For namespaced resources with a target namespace, scope the client to that namespace. + // Otherwise, return a cluster-scoped client (needed for listing across all namespaces). if scope == kubebindv1alpha2.NamespacedScope && namespace != "" { return client.Namespace(namespace) } return client }contrib/kcp/test/e2e/backend.go (1)
64-64: Optional: Document why server reference is unused.The second return value from
StartBackend(*backend.Server) is intentionally unused in this test context. Consider adding a brief comment to make this explicit.+ // Server reference not needed in this test; we only track the address addr, _ := framework.StartBackend(t,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
Makefile(2 hunks)contrib/kcp/README.md(5 hunks)contrib/kcp/go.mod(4 hunks)contrib/kcp/test/e2e/backend.go(1 hunks)contrib/kcp/test/e2e/browser.go(1 hunks)contrib/kcp/test/e2e/kcp.go(1 hunks)contrib/kcp/test/e2e/kcp_test.go(1 hunks)contrib/kcp/test/e2e/logwriter.go(1 hunks)test/e2e/bind/happy-case_test.go(1 hunks)test/e2e/framework/backend.go(2 hunks)test/e2e/framework/dex.go(6 hunks)test/e2e/framework/dex_linux.go(1 hunks)test/e2e/framework/kubeconfig.go(2 hunks)test/e2e/framework/logs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/framework/kubeconfig.go
- test/e2e/framework/dex.go
- contrib/kcp/test/e2e/logwriter.go
- contrib/kcp/test/e2e/browser.go
- test/e2e/framework/dex_linux.go
- contrib/kcp/test/e2e/kcp.go
- Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:55:45.035Z
Learnt from: ntnn
PR: kube-bind/kube-bind#333
File: contrib/kcp/test/e2e/kcp_test.go:270-279
Timestamp: 2025-10-01T15:55:45.035Z
Learning: In KCP, when constructing kubeconfig server URLs for workspaces, the correct pattern is to append ":" + workspace.Base() to an existing server URL that already targets the root workspace, rather than using "/clusters/" + workspace.String(). This colon-separated syntax is KCP's valid format for workspace targeting.
Applied to files:
contrib/kcp/test/e2e/kcp_test.go
🧬 Code graph analysis (4)
test/e2e/bind/happy-case_test.go (1)
test/e2e/framework/backend.go (2)
InstallKubebindCRDs(38-50)StartBackend(52-63)
test/e2e/framework/backend.go (3)
deploy/crd/bootstrap.go (1)
Create(81-83)backend/options/options.go (2)
NewOptions(79-100)ExtraOptions(42-64)test/e2e/framework/dex.go (1)
CreateDexClient(90-124)
contrib/kcp/test/e2e/backend.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
InformerScope(47-47)test/e2e/framework/backend.go (1)
StartBackend(52-63)
contrib/kcp/test/e2e/kcp_test.go (6)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
InformerScope(47-47)test/e2e/framework/dex.go (1)
StartDex(53-88)test/e2e/framework/kcp.go (1)
WithName(59-64)test/e2e/framework/konnector.go (1)
StartKonnector(35-75)test/e2e/framework/apply.go (1)
ApplyFiles(46-69)test/e2e/framework/clients.go (1)
DynamicClient(31-35)
🪛 markdownlint-cli2 (0.18.1)
contrib/kcp/README.md
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: go-test
🔇 Additional comments (8)
test/e2e/bind/happy-case_test.go (1)
67-71: LGTM! Proper CRD installation and backend startup sequence.The changes correctly:
- Install kubebind CRDs after workspace creation and before backend startup
- Use the updated
StartBackendsignature with kubeconfig passed as a flagThis aligns well with the framework refactoring in
test/e2e/framework/backend.go.contrib/kcp/test/e2e/kcp_test.go (3)
41-49: LGTM! Well-structured parallel test organization.The test functions properly delegate to
testKcpIntegrationwith different informer scopes, and uset.Parallel()for efficient test execution.
51-153: LGTM! Comprehensive KCP integration test implementation.The test orchestrates a complete end-to-end flow:
- Dex and KCP server bootstrap
- Consumer/provider workspace setup
- Backend initialization with proper health checks
- API binding and export configuration
- Browser-based binding simulation
- Resource synchronization validation
The cluster ID extraction at lines 126-134 now includes proper validation to prevent panics, addressing previous review feedback.
165-243: LGTM! Thorough resource synchronization testing.The function tests the complete resource lifecycle:
- Create downstream → sync upstream
- Update downstream → sync upstream
- Delete downstream → delete upstream
The subtests are intentionally sequential and dependent, which is appropriate for an integration test validating the full binding workflow.
contrib/kcp/test/e2e/backend.go (1)
37-88: LGTM! Solid backend bootstrap implementation.The function correctly:
- Retrieves the API export endpoint URL with proper error handling
- Creates the backend kubeconfig targeting the kube-bind workspace
- Starts the backend with appropriate KCP-specific flags
- Validates backend readiness using an HTTP health check with context
The health check at lines 75-84 properly uses
http.NewRequestWithContextinstead of external tools, which was correctly addressed from previous feedback.test/e2e/framework/backend.go (3)
38-50: LGTM! Proper CRD installation helper.The function correctly uses
t.Context()for the CRD creation calls, which is valid sincetesting.TB.Context()has been available since Go 1.24 (as confirmed in previous discussions).
52-63: Excellent security improvement: dynamic key generation.The refactored
StartBackendnow generates ephemeral signing and encryption keys per test usingsecurecookie.GenerateRandomKey, eliminating the hard-coded secrets that triggered security scanners in previous versions. This addresses the critical security concern from prior reviews.
65-106: LGTM! Robust backend initialization with dynamic provisioning.The function now:
- Dynamically provisions Dex clients with ephemeral credentials via
CreateDexClient- Properly validates configuration with
completed.Validate()at line 93- Sets
TestingSkipNameValidationto avoid metric name conflicts in multi-controller scenarios- Maintains proper lifecycle management with context cancellation
This comprehensive refactoring significantly improves test reliability and security by eliminating hard-coded credentials and port conflicts.
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
2 participants
Summary
Adds an e2e test for kcp.
What Type of PR Is This?
/kind feature
Related Issue(s)
Fixes #
Release Notes
Summary by CodeRabbit
Documentation
Tests
Chores